-
Notifications
You must be signed in to change notification settings - Fork 150
Minor validation logic improvements #782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
No more flaky tests? 😄 Hm, looks like this is still related? Is it just ACK (202)?
|
Sources/LiveKit/Support/HTTP.swift
Outdated
case valid | ||
case invalid(message: String) | ||
// Network error etc. | ||
case unknown(error: Error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO either throws
/Result
/Error
where throws
should be prioritized
Let's unblock this PR, 2 nitpicks:
Othwerwise, LGTM 🌮 |
case webRTC = 201 | ||
|
||
case network // Network issue | ||
case validation // Network issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd rename that to authorization
(or better?), as this is very generic (not sure about other SDKs).
Otherwise LGTM^2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we used to call this "validation", the endpoint (path is /validate)
pathSegments.append("validate") |
So maybe validation is better ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that matches backend, then why not
Throws different type of error if server validation failed